-
Notifications
You must be signed in to change notification settings - Fork 7.8k
drivers: dma_mcux_edma: Simple Refactoring #92869
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@decsny it does not well on my mimxrt1060_evk@c platform.
and for uart tests/drivers/uart/uart_async_api/drivers.uart.async_api.lpuart.rt_nocache
|
@hakehuang it looks like there was a typo in a #if condition which affected platform with cache like RT1060, should be fixed now |
@decsny now regression pass on v4.2.0-rc3-26-g6aea2db65c32, but need fix issue with
|
@decsny please address the CI issues |
i will when i have time |
The dependency on the chosen node for dtcm can be expressed in Kconfig language. Cache we care about is CPU DCACHE, not the meaningless "MCUX Cache" The macros can be reordered to be simpler by having only one level of conditional (no nesting) instead of three levels. Move this code closer in the file to where this cache attribute macro is actually going to be used (the init macro) instead of randomly splitting up the struct definitions at the top. Signed-off-by: Declan Snyder <[email protected]>
One #ifdef outside the functions is simpler to read than two #ifdefs inside the functions. Signed-off-by: Declan Snyder <[email protected]>
Instead of having preprocessor code, make a hidden kconfig to indicate this and be smarter about the C code (these are all powers of two) Signed-off-by: Declan Snyder <[email protected]>
There is two completely different types of reload modes happening here, therefore we should split this function into two completely separate functions because it was getting large and hard to read. Removes one level of indentation. Signed-off-by: Declan Snyder <[email protected]>
There was actually three different types of configuration modes happening here, and this function was getting extremely bulky and hard to read due to the amount of nesting of conditionals. Split these into separate functions and call them appropriately depending on the type of transfer. Signed-off-by: Declan Snyder <[email protected]>
Extract and group dmamux related code together for readability. Signed-off-by: Declan Snyder <[email protected]>
Halve all this ifdef code by making a macro function for the memory mapping. Signed-off-by: Declan Snyder <[email protected]>
|
@hakehuang can you help retesting this |
@decsny sorry for late response, the regression test pass on v4.2.0-1410-geda55cb99495 no regression issues found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some nits
static int dma_mcux_edma_configure_basic(const struct device *dev, | ||
uint32_t channel, | ||
struct dma_config *config, | ||
edma_transfer_type_t transfer_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: indentation
static void edma_configure_dmamux(const struct device *dev, uint32_t channel, | ||
struct dma_config *config, edma_transfer_type_t transfer_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
#define edma_configure_dmamux(...) | ||
#define edma_log_dmamux(...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to define these as (void)0
@decsny , sorry, due to my mail rules setting issue, I miss this build failure on mimxrt1180_evk platforms, thanks @lucien-nxp find this, I reported in #94539 |
This edma driver is extremely important for NXP platforms as it is used by many different drivers itself, and what I am noticing is that is always causing a lot of problems to debug through it because it is a mess. So this PR is doing a basic refactor which should not change any logic of the program flow but just splitting things into helper functions to be more readable, etc.